feat: Support OpenAPI 3.1.x - #1231
Conversation
CharlesDuboisSAP
left a comment
CharlesDuboisSAP
left a comment
There was a problem hiding this comment.
Some test coverage is lacking, you could add some more unit tests
…k-java into support-openapi-3.1
There was a problem hiding this comment.
Overall LGTM, one small thing:
Why did you duplicate all integrations tests in DataModelGeneratorApacheIntegrationTest? I would remove them since they are already tested in DataModelGeneratorIntegrationTest, and they cover the same lines of code, apache only changes the inside of the Api class which are not the point of your tests anyway.
ZhongpinWang
changed the title
|
@CharlesDuboisSAP @Jonas-Isr Can you add a simple release note for this? I don't know how you do this normally in Java team. |
|
I added a log warning for 3.1 to point out that this is an experimental feature. We can remove this after a while. |
CharlesDuboisSAP
left a comment
CharlesDuboisSAP
left a comment
There was a problem hiding this comment.
LGTM, let's wait for @Jonas-Isr's review
Jonas-Isr
left a comment
Jonas-Isr
left a comment
There was a problem hiding this comment.
Looks good to me :)
I only had some pretty minor points.
| s -> s != null && s.getProperties() != null && s.getProperties().remove(propertyName) != null; | ||
| s -> s.getProperties() != null && s.getProperties().remove(propertyName) != null; |
There was a problem hiding this comment.
(Question)
Why is it save to remove the null check here?
| // warn when deprecated nullable: true is used in an OAS 3.1 spec | ||
| if( isOas31 && schema.getNullable() != null ) { | ||
| LOGGER | ||
| .warn( | ||
| "'nullable: true' is not a valid OAS 3.1 keyword on $ref schema '{}'. " | ||
| + "Use anyOf: [{{$ref: \"...\"}}, {{type: \"null\"}}] instead.", | ||
| schema.get$ref()); | ||
| } | ||
|
|
There was a problem hiding this comment.
(Minor)
Maybe we should test this path as well?
| } | ||
| } | ||
|
|
||
| @SuppressWarnings( { "rawtypes", "RedundantSuppression" } ) |
There was a problem hiding this comment.
(Question)
Is the SurpressWarnings actually necessary here? It looks to me like this method would be fine without it.
| @SuppressWarnings( { "rawtypes", "RedundantSuppression" } ) |
3 participants
Context
This is a generated (later manually reviewed and fixed) PoC for evaluating the effort of supporting OpenAPI 3.1.x in the generator.
The following two files coming from the initial round of generation might be outdated but you can use as a reference.
I generated all data models in AI SDK Java with the new SNAPSHOT cloud sdk generator, and there were no breaking changes.
See SAP/ai-sdk-java#958 for the generated result.